Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(exceptions): separate failed signin error #1478

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

jorwoods
Copy link
Contributor

Closes #1472

This makes sign in failures their own class of exceptions, while still inheriting from NotSignedInException to not break backwards compatability for any existing client code. This should allow users to get out more specific exceptions more easily on what failed with their authentication request.

@jorwoods jorwoods force-pushed the jorwoods/signin_error branch 2 times, most recently from 878e813 to 74f7797 Compare September 28, 2024 17:07
@bcantoni
Copy link
Contributor

@jorwoods do you think we should/could incorporate a change here for the minor issue #1450? I don't know whether that other issue is big enough to fix, but since your change here is in a related area maybe we could do together.

@jorwoods
Copy link
Contributor Author

@bcantoni I made a tweak to how the ServerInfoItem does its error handling to raise the error immediately. It currently raises a NonXMLResponseError. Not sure if you would want something custom raised there to indicate that the problem is more extensive beyond it not being valid XML.

@jorwoods
Copy link
Contributor Author

I also changed the log level on those messages from info to exception so that they'll bubble up more clearly in logs.

Closes tableau#1472

This makes sign in failures their own class of exceptions, while still
inheriting from NotSignedInException to not break backwards
compatability for any existing client code. This should allow users
to get out more specific exceptions more easily on what failed with
their authentication request.
If ServerInfoItem.from_response gets invalid XML, raise the error
immediately instead of suppressing the error and setting an invalid
version number
@jorwoods jorwoods force-pushed the jorwoods/signin_error branch from 388d1bd to 6eab3c9 Compare September 28, 2024 18:57
@bcantoni
Copy link
Contributor

Did some quick testing and this change looks good.

Sign in with incorrect PAT, before:

tableauserverclient.server.endpoint.exceptions.NotSignedInError: (b'<?xml version=\'1.0\' encoding=\'UTF-8\'?><tsResponse xmlns="http://tableau.com/api" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://tableau.com/api https://help.tableau.com/samples/en-us/rest_api/ts-api_3_25.xsd"><error code="401001"><summary>Signin Error</summary><detail>The personal access token you provided is invalid.</detail></error></tsResponse>', 'https://MYSERVER/api/3.25/auth/signin')

Sign in with incorrect PAT, after:

tableauserverclient.server.endpoint.exceptions.FailedSignInError: Failed Sign In Error:

	401001: Signin Error
		The personal access token you provided is invalid.

Copy link
Contributor

@bcantoni bcantoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought I don't think we need to do anything specific for 1450 (no specific error for pointing to a non-Tableau server).

@jorwoods
Copy link
Contributor Author

Do you want it to raise the NonXMLResponseError? Or should I revert that change?

@jacalata jacalata merged commit b49eac5 into tableau:development Oct 10, 2024
22 checks passed
@jorwoods jorwoods deleted the jorwoods/signin_error branch October 11, 2024 21:39
jacalata pushed a commit that referenced this pull request Oct 24, 2024
* Feature: export custom views #999 (#1506)
* feat(exceptions): separate failed signin error (#1478)
* refactor request_options, add language param (#1481)
* Set FILESIZE_LIMIT_MB via environment variables (#1466)
* added PulseMetricDefine cap (#1490)
* Adding project permissions handling for databases, tables and virtual connections (#1482)
* fix: queryset support for flowruns (#1460)
* fix: set unknown size to sys.maxsize
* fix: handle 0 item response in querysets (#1501)
* chore: support VizqlDataApiAccess capability (#1504)
* refactor(test): extract error factory to _utils
* chore(typing): flowruns.cancel can also accept a FlowRunItem
* chore: type hint default permissions endpoints (#1493)
* chore(versions): update remaining f-strings (#1477)
* Make urllib3 dependency more flexible (#1468)
* Update requests library for CVE CVE-2024-35195 (#1507)

* chore(versions): Upgrade minimum python version (#1465)
* ci: cache dependencies for faster builds (#1497)
* ci: build on python 3.13 (#1492)
* Update samples for Python 3.x compatibility (#1479)
* chore: remove  py2 holdover code (#1496)
* #Add 'description' to datasource sample code (#1475)
* Remove sample code showing group name encoding (#1486)
* chore(typing): include samples in type checks (#1455)

* fix: docstring on QuerySet
* docs: add docstrings to auth objects and endpoints (#1484)
* docs: docstrings for Server and ServerInfo (#1494)
* docs: docstrings for user item and endpoint (#1485)
* docs: docstrings for site item and endpoint (#1495)
* docs: workbook docstrings (#1488)
* #1464 - docs update for filtering on boolean values (#1471)

---------
Co-authored-by: Brian Cantoni <[email protected]>
Co-authored-by: Jordan Woods <[email protected]>
Co-authored-by: Jordan Woods <[email protected]>
Co-authored-by: Jac <[email protected]>
Co-authored-by: Henning Merklinger <[email protected]>
Co-authored-by: AlbertWangXu <[email protected]>
Co-authored-by: TrimPeachu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants